FINERACT-2293: Migrate client address module to new command pipeline#5657
FINERACT-2293: Migrate client address module to new command pipeline#5657avivijay19 wants to merge 1 commit intoapache:developfrom
Conversation
...c/main/java/org/apache/fineract/portfolio/address/service/ClientAddressWriteServiceImpl.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/apache/fineract/portfolio/address/service/ClientAddressWriteServiceImpl.java
Outdated
Show resolved
Hide resolved
...rovider/src/main/java/org/apache/fineract/portfolio/client/api/ClientAddressApiResource.java
Outdated
Show resolved
Hide resolved
avivijay19
force-pushed
the
FINERACT-2293
branch
2 times, most recently
from
602cd1a to
91c3b39
Compare
...in/java/org/apache/fineract/portfolio/address/handler/ClientAddressCreateCommandHandler.java
Outdated
Show resolved
Hide resolved
...in/java/org/apache/fineract/portfolio/address/handler/ClientAddressCreateCommandHandler.java
Outdated
Show resolved
Hide resolved
...in/java/org/apache/fineract/portfolio/address/handler/ClientAddressUpdateCommandHandler.java
Outdated
Show resolved
Hide resolved
...provider/src/main/java/org/apache/fineract/portfolio/address/service/AddressReadService.java
Outdated
Show resolved
Hide resolved
...ider/src/main/java/org/apache/fineract/portfolio/address/service/AddressReadServiceImpl.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/org/apache/fineract/portfolio/address/service/ClientAddressWriteService.java
Outdated
Show resolved
Hide resolved
avivijay19
force-pushed
the
FINERACT-2293
branch
from
371162c to
f2556aa
Compare
| public interface ClientAddressWriteService { | ||
|
|
||
| CommandProcessingResult addClientAddress(Long clientId, Long addressTypeId, JsonCommand command); | ||
| ClientAddressCreateResponse createClientAddress(ClientAddressCreateRequest request); |
There was a problem hiding this comment.
What is the difference between createClientAddress and addNewClientAddress? Only difference I see is in one you pass an client ID, in the other the whole client object? On the surface looks like this is doing the samem? can we eliminate one?
| } | ||
|
|
||
| @Override | ||
| public void addNewClientAddress(Client client, List<ClientAddressCreateRequest> requests) { |
There was a problem hiding this comment.
Why do we need this loop inside this service? I bet this is called only in 1 place... let's create the loop there... no added value hiding 3 lines of code in a service and they add basically no new functionality. We are not saving anything here... unless you tell me we are calling this function in 57 places...
| } | ||
| } | ||
|
|
||
| private Address createAddressFromRequest(ClientAddressCreateRequest request) { |
There was a problem hiding this comment.
Can move to createClientAddres function once we have remove that other one with the loop.
| import org.springframework.jdbc.core.JdbcTemplate; | ||
|
|
||
| @Configuration | ||
| public class AddressConfiguration { |
There was a problem hiding this comment.
ClientAddressConfiguration.... consistency...
|
|
||
| @Path("/v1/client") | ||
| @Component | ||
| @Tag(name = "Clients Address", description = "Address module is an optional module and can be configured into the system by using GlobalConfiguration setting: enable-address. In order to activate Address module, we need to enable the configuration, enable-address by setting its value to true.") |
There was a problem hiding this comment.
Description is maybe better in triple quotes aka multi-line strings. This here won't pass the Linter (aka Spotless plugin), whereas a multi-line string allows you to make this readable for everyone and the linter won't touch it.
| @Consumes({ MediaType.APPLICATION_JSON }) | ||
| @Produces({ MediaType.APPLICATION_JSON }) | ||
| @Operation(summary = "Retrieve client address template", operationId = "retrieveTemplateClientAddress") | ||
| public AddressData getAddressesTemplate() { |
There was a problem hiding this comment.
ClientAddressData... consistency
| @@ -23,7 +23,7 @@ | |||
| import java.util.HashMap; | |||
There was a problem hiding this comment.
As soon as we read "XXXSwagger.java" something is wrong aka not finished. Again, these are the dummy classes we intend to remove... mentioned it already a couple of times... maybe go slower with the changes and double check yourself before you submit.
|
|
||
| if (isAddressEnabled) { | ||
| this.addressWritePlatformService.addNewClientAddress(newClient, command); | ||
| final com.google.gson.JsonArray addressArray = command.arrayOfParameterNamed("address"); |
There was a problem hiding this comment.
This is not type safety! This kind of thing is now consistently slipping through the cracks. ANYTHING:
- GSON
- xxxParamterNamed
- JsonArray
- JsonXXX
- JsonCommand
- CommandWrapper
- CommandBuilder
... needs to go, because this is the goal of this exercise, to make things type safe... all of the above is not. Again, maybe go a bit slower... otherwise this feels a bit like a prompt loop. I don't mind reviewing... but let's keep an eye on these details.
2 participants
Description
Describe the changes made and why they were made. (Ignore if these details are present on the associated Apache Fineract JIRA ticket.)
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Your assigned reviewer(s) will follow our guidelines for code reviews.